Add rename_view to REST Catalog - #2149
Conversation
|
@Fokko @kevinjqliu mind taking a look when you can? thanks! |
f4e2a61 to
756ee0b
Compare
756ee0b to
8ad2e55
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
8ad2e55 to
9188ced
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
c8996c2 to
7f39e33
Compare
|
@geruh looks like you've been going through the View PRs. Mind taking a look at this one? Follows the others near-identically. |
|
@ebyhr can you take a look? You've been doing a lot of work around views lately. |
|
@kevinjqliu @Fokko @geruh mind taking a look? This will help with all of the View work that's been done. |
abnobdoss
left a comment
abnobdoss
left a comment
There was a problem hiding this comment.
Looks good! The only concern I have is the point you highlighted in the PR description of the discrepancy between rename_table returning a Table but rename_view returning None. It sounds like the right fix would be to change rename_table to return None to be aligned to the spec. Since that is a breaking change I imagine we likely would have to wait till 1.0.0 to align those functions.
|
I agree that |
|
@kevinjqliu mind taking a look? This should be good to go! |
8a67d40 to
e88fe55
Compare
| @retry(**_RETRY_ARGS) | ||
| def rename_view(self, from_identifier: str | Identifier, to_identifier: str | Identifier) -> None: | ||
| self._check_endpoint(Capability.V1_RENAME_VIEW) | ||
| payload = { |
There was a problem hiding this comment.
self._split_identifier_for_json is still called twice per namespace.
I think we can move the payload after checking namespace_exists, for example
source = self._split_identifier_for_json(from_identifier)
destination = self._split_identifier_for_json(to_identifier)
# Ensure that namespaces exist on source and destination.
source_namespace = source["namespace"]
if not self.namespace_exists(source_namespace):
raise NoSuchNamespaceError(f"Source namespace does not exist: {source_namespace}")
destination_namespace = destination["namespace"]
if not self.namespace_exists(destination_namespace):
raise NoSuchNamespaceError(f"Destination namespace does not exist: {destination_namespace}")
payload = {
"source": source,
"destination": destination,
}
6 participants
Rationale for this change
As part of #818, this adds
rename_viewsupport to the Iceberg REST Catalog.The REST Catalog spec specifies that both
rename_viewandrename_tabledo not return anything. Currently,rename_tablereturns the table. I'm happy to change that API if we want, but it would technically be a breaking change.Are these changes tested?
Added unit tests.
Are there any user-facing changes?
rename_viewsupport to Iceberg REST Catalog.